feat(coprocessor): add local coverage tooling and pre-push hook#2095
feat(coprocessor): add local coverage tooling and pre-push hook#2095PanGan21 wants to merge 7 commits intopanos/add-coverage-toolfrom
Conversation
|
@claude /pr-review |
|
Claude timed out while processing the request. — View run |
|
@claude /pr-review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
[AUTOMATED]
Code Review
Overall this is a well-structured PR that adds useful local coverage tooling. The shell scripts are defensive and the non-blocking hook design is sensible. However, there is one significant issue with how the new hooks system interacts with the existing one.
Checked for: bugs, guideline compliance, error handling, and security.
Issues found: 2 (1 high, 1 low)
.githooks/install.sh
Outdated
| # Sets up .githooks/ as the git hooks directory for this repository. | ||
| # | ||
| cd "$(git rev-parse --show-toplevel)" || exit 1 | ||
| git config core.hooksPath .githooks |
There was a problem hiding this comment.
[AUTOMATED]
Setting core.hooksPath silently disables existing git hooks (confidence: 97/100)
git config core.hooksPath .githooks causes Git to exclusively use .githooks/ for hook resolution, completely bypassing .git/hooks/. This means any hooks previously installed via .github/hooks/install.sh are silently disabled:
.github/hooks/pre-push(blocking): runscargo fmt --check,cargo clippy -- -D warnings, andcargo test— these quality gates are lost.github/hooks/commit-msg: enforces Angular conventional commit format — also lost
The new .githooks/pre-push does not chain to or invoke the old hooks, and .githooks/ has no commit-msg hook at all. A developer who previously had the old hooks installed and follows the README to install these new hooks will unknowingly lose their quality gates.
Suggested fix (pick one):
-
Consolidate (recommended): Add the coverage logic to the existing
.github/hooks/pre-pushinstead of creating a parallel hooks system. -
Chain hooks: Have
.githooks/pre-pushinvoke the old hook first:OLD_HOOK="$(git rev-parse --show-toplevel)/.github/hooks/pre-push" if [ -x "$OLD_HOOK" ]; then "$OLD_HOOK" "$@" || exit $? fi
And add a
commit-msgpassthrough in.githooks/as well. -
At minimum: Warn the user in
install.shand the README that this replaces the default hooks directory.
There was a problem hiding this comment.
Since the existing githook for fmt and clippy seems broken I keep the existing directory and only warn the user about overriding.
There was a problem hiding this comment.
Added both hooks in the same directory and the developer has a choice to install one githook or both.
|
Review posted successfully. Here's a summary: PR Review Summary — #2095PR: feat(coprocessor): add local coverage tooling and pre-push hook Overall this is a well-structured PR with useful local coverage tooling. The shell scripts are defensive, the non-blocking hook design is sensible, and the auto-detection of changed crates is a nice workflow improvement. Issues Found: 2
The main concern is that |
…nd handle empty report time
…the same directory
PanGan21
force-pushed
the
panos/pre-push-coverage
branch
from
90470de to
c5aa5a2
Compare
|
For this to be effective in the development workflow, the pre-push hook would need to be enforced. Otherwise, running it would remain optional for developers. Given that, this PR doesn’t seem to provide much value at the moment and would mainly add additional local execution time. For that reason, I’m closing the PR. |
🧪 CI InsightsHere's what we observed from your CI run for c5aa5a2. 🟢 All jobs passed!But CI Insights is watching 👀 |
1 participant
Summary
make coverage), single crate (make coverage-crate CRATE=<name>), or only changed crates (make coverage-changed)make coverage-changedcompares against main, detects which crates have changes, and runs coverage only for thosesh .githooks/install.sh/sh .githooks/uninstall.sh)Developer workflow
./githooks/install.shonce to enable the pre-push hookmake coverage-changedincoprocessor/fhevm-engine/— it auto-detects which crates changed vs main and suggests to run coverage only for thosegit push, the hook checks for a fresh coverage report. If missing or stale, it prints a warning (never blocks the push)Ouput